Maintain replication connection between sync flows #1211
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
serprex
commented
Feb 6, 2024
serprex
force-pushed
the
spiritus-mundi
branch
3 times, most recently
from
c936149 to
772def2
Compare
serprex
force-pushed
the
spiritus-mundi
branch
3 times, most recently
from
c00b723 to
e14b538
Compare
serprex
changed the title
|
Kevin pointed out a problem: in non parallel sync-normalize long normalize will have connection silent for too long & postgres can drop connection. Need to have keepalive logic. MaintainPull could work, but then we need to make sure to use synchronization between MaintainPull & StartFlow |
serprex
force-pushed
the
spiritus-mundi
branch
3 times, most recently
from
a395c50 to
7cdb75b
Compare
iskakaushik
reviewed
Feb 9, 2024
iskakaushik
reviewed
Feb 9, 2024
| @@ -45,9 +45,10 @@ type SlotSnapshotSignal struct { | |||
| type FlowableActivity struct { | |||
| CatalogPool *pgxpool.Pool | |||
| Alerter *alerting.Alerter | |||
| CdcCacheRw sync.RWMutex | |||
There was a problem hiding this comment.
Maybe have a replication connection manager struct that takes care of:
- This map and locking.
- Keeping track of the connection health and lifecycle.
- Any other additional metadata pertaining to the replication connection.
There was a problem hiding this comment.
Tracking connection health & lifecycle are part of MaintainPull which needs to exist either way to keep heartbeating session. Additional metadata belongs in the connector, where we avoid contention on CdcCacheRw
I could see moving replState/replConn out of the connector, then storing a struct { replState, replConn, connector } as the value of the hashmap. But for now putting it all in connector works about the same
iskakaushik
reviewed
Feb 9, 2024
serprex
force-pushed
the
spiritus-mundi
branch
from
c30bd20 to
8124726
Compare
Merged
iskakaushik
pushed a commit
that referenced
this pull request
Feb 10, 2024
Required updating connector interfaces. Bit annoying `ctx` everywhere, but that's ultimately the correct way. Was running into context complications in #1211 with connector being shared between activities Putting context in struct essentially makes that struct a context, but this is not the context we necessarily want. For more context, see https://zenhorace.dev/blog/context-control-go Some changes were made: 1. GetCatalog takes a context now instead of using `context.Background()` 2. eventhubs processBatch now takes context instead of using `context.Background()` 3. many instances of `Query`/`Exec` in snowflake/clickhouse converted to `QueryContext`/`ExecContext` 4. got rid of cancel context in ssh tunnel, context being passed in is sufficient Followup to #1238
serprex
force-pushed
the
spiritus-mundi
branch
6 times, most recently
from
7558024 to
ff908e5
Compare
iskakaushik
reviewed
Feb 16, 2024
serprex
force-pushed
the
spiritus-mundi
branch
3 times, most recently
from
4ccc9dd to
7583072
Compare
|
does using |
|
Recreation will have new RunID. OriginalRunID is because RunID changes with replays, OriginalRunID maintains determinism https://pkg.go.dev/go.temporal.io/[email protected]/internal#WorkflowInfo
|
iskakaushik
reviewed
Feb 16, 2024
iskakaushik
reviewed
Feb 16, 2024
serprex
force-pushed
the
spiritus-mundi
branch
from
e6857ea to
5bdece2
Compare
serprex
force-pushed
the
spiritus-mundi
branch
2 times, most recently
from
92a9064 to
9010c35
Compare
serprex
added a commit
that referenced
this pull request
Feb 20, 2024
Less side effects, less error handling, can correlate different workflows with same run id Also pull in some other cleanup from #1211
serprex
added a commit
that referenced
this pull request
Feb 20, 2024
Less side effects, less error handling, can correlate different workflows with same run id Also pull in some other cleanup from #1211
serprex
added a commit
that referenced
this pull request
Feb 20, 2024
Less side effects, less error handling, can correlate different workflows with same run id Also pull in some other cleanup from #1211
serprex
force-pushed
the
spiritus-mundi
branch
from
9010c35 to
265fa5c
Compare
serprex
added a commit
that referenced
this pull request
Feb 21, 2024
Only pass config & options to StartFlow, removing StartFlowInput In fact, while we're at it, rename StartFlow to SyncFlow, the name doesn't really make sense anymore, & it'll make less sense after #1211
serprex
added a commit
that referenced
this pull request
Feb 21, 2024
Only pass config & options to StartFlow, removing StartFlowInput In fact, while we're at it, rename StartFlow to SyncFlow, the name doesn't really make sense anymore, & it'll make less sense after #1211
serprex
added a commit
that referenced
this pull request
Feb 21, 2024
Only pass config & options to StartFlow, removing StartFlowInput In fact, while we're at it, rename StartFlow to SyncFlow, the name doesn't really make sense anymore, & it'll make less sense after #1211
serprex
force-pushed
the
spiritus-mundi
branch
from
265fa5c to
0d21eb8
Compare
iskakaushik
reviewed
Feb 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently we reconnect with each sync flow, requiring repeatedly starting replication. This can take an exceedingly long time for some workloads on some databases
Fix: use temporal session to share state between activities, use a single source connector throughout cdc flow, & move replication connection back into source connection